New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[parser] Invalid NonOctal Decimal #10467
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11647/ |
if (isFloat || octal) this.raise(start, "Invalid BigIntLiteral"); | ||
// disallow floats, legacy octal syntax and non octal decimals | ||
// new style octal ("0o") is handled in this.readRadixNumber | ||
if (isFloat || octal || isNonOctalDecimal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does isNonOctalDecimal == true
imply (octal || isNonOctalDecimal) == true
? If so we could consider removing octal
check here.
octal = false; | ||
} | ||
if (/[89]/.test(number)) octal = false; | ||
if (/^[0-9]*$/.test(number)) isNonOctalDecimal = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
octal == true
implies that startsWithDot == false
(because it starts with zero), which in turn means that this.readInt(10) != null
, or otherwise we would throw Invalid number
error here.
if (!startsWithDot && this.readInt(10) === null) {
this.raise(start, "Invalid number");
}
IIRC this.readInt(10)
would ensure the input from start
to this.state.pos
to always be [0-9]
, if so we don't have to check [0-9]
again, which means isNonOctalDecimal
equals to the previous definition of octal
:
this.state.pos - start >= 2 && this.input.charCodeAt(start) === charCodes.digit0;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so it would be enought to do
if (/[89]/.test(number)) {
octal = false;
isNonOctalDecimal = true;
}
So that isNonOctalDecimal
literally means "we tought that it was an octal number, but it turns out that it isn't"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, i didn't see it in that way, it took me a while to realize why the test i added was passing, but it's because when octal
is set to false here:
if (/[89]/.test(number)) octal = false;
isNonOctalDecimal
remains true, so it can throw the error here:
if (isFloat || octal || isNonOctalDecimal) {...}
It's confusing, thanks for the review, i'll come back with some changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolo-ribaudo Nice! Also, the decimal
part isn't confusing? Sounds like fractional numbers, maybe it can only be isNonOctal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec calls it NonOctalDecimalIntegerLiteral
, so maybe nonOctalDecimalInt
?
But if you prefer isNonOctal
, it's ok 👍
if (isFloat || octal) this.raise(start, "Invalid BigIntLiteral"); | ||
// disallow numeric separators in non octal decimals | ||
if (this.hasPlugin("numericSeparator") && isNonOctalDecimalInt) { | ||
const underscorePos = this.input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this whole if
out of the if (this.hasPlugin("bigInt"))
? It also applies to normal numbers:
0_1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you run make test-test262-update-whitelist
again and add a test for 0_8
which is now correctly disallowed by this PR?
Now all green again 🎉 |
.indexOf("_"); | ||
if (underscorePos > 0) { | ||
this.raise( | ||
underscorePos, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be start + underscorePos
?
void 0_n;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolo-ribaudo will resolve the conflicts.
language/literals/bigint/numeric-separators/numeric-separator-literal-nonoctal-09-err.js(default) | ||
language/literals/bigint/numeric-separators/numeric-separator-literal-nonoctal-0_8-err.js(default) | ||
language/literals/bigint/numeric-separators/numeric-separator-literal-nonoctal-0_9-err.js(default) | ||
language/literals/bigint/numeric-separators/numeric-separator-literal-nzd-nsl-dds-leading-zero-err.js(default) | ||
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-00-err.js(default) | ||
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-01-err.js(default) | ||
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-07-err.js(default) | ||
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-0_0-err.js(default) | ||
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-0_1-err.js(default) | ||
language/literals/numeric/numeric-separators/numeric-separator-literal-lol-0_7-err.js(default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to open another PR to fix these tests, you are welcome to do so!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, i would like to do so! Should i make an issue or just as a follow up of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naa, just a PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goodd 👌
Thanks guys! Appreciate the review, I learned a lot 🙏 |
I added a regex verification for a NonOctal Decimal Integer, defined as a sequence of decimal digits starting with 0 and with at least two characters, i did it within the scope of the
if (octal)
becauseoctal
ensure two of the requirements, and there was a similar octal verification.Todo: